Domainmapping bring your own certificate by leetcope · Pull Request #11250 · knative/serving

leetcope · 2021-04-22T17:34:53Z

Allow domainmappings to specify the tls secret to be used by the autoTLS
certificate

These changes assume that autoTLS is enabled in the cluster and that if the secret doesn't exist in the cluster the intent
is to create a new secret with the given name

Fixes #10530

Release Note

Domainmapping can now specify a tls secret to be used as the https certificate

codecov · 2021-04-22T17:39:33Z

Codecov Report

Merging #11250 (7461340) into main (3676663) will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main   #11250      +/-   ##
==========================================
+ Coverage   87.70%   87.71%   +0.01%     
==========================================
  Files         191      191              
  Lines        9246     9256      +10     
==========================================
+ Hits         8109     8119      +10     
  Misses        882      882              
  Partials      255      255

Impacted Files	Coverage Δ
...g/apis/serving/v1alpha1/domainmapping_lifecycle.go	`81.81% <0.00%> (-3.90%)`	⬇️
pkg/apis/serving/v1alpha1/domainmapping_types.go	`100.00% <ø> (ø)`
pkg/reconciler/domainmapping/reconciler.go	`92.10% <100.00%> (+0.43%)`	⬆️
pkg/activator/net/revision_backends.go	`92.41% <0.00%> (+0.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3676663...7461340. Read the comment docs.

julz

This looks good, thanks! 🎉 I mostly just have a little bike-shed about the field name :).

I guess (probably in a follow up, no need for it to be done in this PR, as far as I'm concerned) we will want to relax the check in the reconciler to allow tls even with autoTLS disabled as long as (and only as long as!) the secret is provided explicitly.

julz · 2021-04-23T20:50:54Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+	// If defined it will use the existing tls secret with the given name, otherwise it will attempt to create
+	// a new secret with the expected name to be used by the certificate.
+	// +optional
+	TLSSecret string `json:"tlsSecret,omitempty"`


What do you think about having this roughly match the similar section in K8s Ingress? (Mostly just for consistency/familiarity). That would also line up with (Knative) IngressTLS which has secretName.

i.e.

tls: secretName: ..

@julz I've addressed your comments please let me know if everything looks good to you

leetcope · 2021-05-03T19:17:00Z

/retest

leetcope · 2021-05-03T19:58:11Z

/assign @dprotaso

julz

Couple little comments

julz · 2021-05-04T09:28:44Z

pkg/reconciler/domainmapping/reconciler.go


 func (r *Reconciler) tls(ctx context.Context, dm *v1alpha1.DomainMapping) ([]netv1alpha1.IngressTLS, []netv1alpha1.HTTP01Challenge, error) {
-	if !autoTLSEnabled(ctx, dm) {
+	if !autoTLSEnabled(ctx, dm) && dm.Spec.TLS == nil {


I'd actually land this line in a follow-on PR I think: we want to think through the cases here (eg if autotls is not enabled and spec.TLS is non-nil but SecretName is empty what should happen? Should we error out if the cert doesn't exist but autotls is disabled?) and probably add some test coverage.

julz · 2021-05-04T09:29:41Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+type SecretTLS struct {
+	// SecretName is the name of a TLS secret.
+	//
+	// Either a existing secret in the cluster or the new secret to be created.


Suggested change

// Either a existing secret in the cluster or the new secret to be created.

// Either an existing secret in the cluster or the new secret to be created.

dprotaso · 2021-05-04T15:53:29Z

pkg/reconciler/route/resources/certificate.go

 		Spec: networkingv1alpha1.CertificateSpec{
 			DNSNames:   []string{dnsName},
-			SecretName: certName,
+			SecretName: secretName,


The godoc for this says

SecretName is the name of the secret resource to store the SSL certificate in.

It's not obvious whether different controllers will check this secret and not overwrite it. Even if it does not what happens when the certificate in the secret expires?

I wonder if when we bring your own certificate we don't use the default certificate class.

I wonder if when we bring your own certificate we don't use the default certificate class.

I guess we still need something to set the certificate as 'ready`

At least in net-http01 I see it checks for the secret before trying to create it here which is consistent with what I've tested, this also happens with cert-manager but I haven't found the specific code that does this in cert-manager. I'm not sure if this answers your question

net-http01 may be work well but we can't assume other's won't

ie. cert manager

The secret name is delegated to a cert manager CRD here. A rough scan looks like there are edge cases where it might fail

Specifically the secret is evaluated against the following policy chain to determine whether to re-issue the certificate

Two concerning assertions are:

SecretPrivateKeyMatchesSpec

When creating the cert manager crd we don't indicate the private key type. When it's not set cert manager assumes we're bringing a RSA private key. Thus if we brought a ECDSA cert then I think it trigger a reissue that'll fail/overwrite our secret

SecretIssuerAnnotationsNotUpToDate

it seems like the secret needs the 'correct' annotations or it'll be re-issued - these annotations are cert manager specific.

dprotaso · 2021-05-06T13:44:55Z

/retest

dprotaso · 2021-05-06T13:45:30Z

Can we add an e2e test?

xtreme-sameer-vohra · 2021-05-06T17:26:42Z

docs/serving-api.md

+<td>
+<em>(Optional)</em>
+<p>TLS indicates the existing or expected tls secret that should be used for certificate generation.</p>
+<p>If defined it will use the existing tls secret with the given name, otherwise it will attempt to create


same comment as below

xtreme-sameer-vohra · 2021-05-06T17:26:55Z

docs/serving-api.md

+<td>
+<em>(Optional)</em>
+<p>TLS indicates the existing or expected tls secret that should be used for certificate generation.</p>
+<p>If defined it will use the existing tls secret with the given name, otherwise it will attempt to create


same comment as below

xtreme-sameer-vohra · 2021-05-06T17:30:20Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+	// TLS indicates the existing or expected tls secret that should be used for certificate generation.
+	//
+	// If defined it will use the existing tls secret with the given name, otherwise it will attempt to create
+	// a new secret with the expected name to be used by the certificate.


Is this still true ? As in will a new secret be created if one doesnt exist ?

Might also want to update the description in the PR and original issue if the requirements have changed as we explicitly don't want to create a secret if it doesn't exist

dprotaso · 2021-05-18T18:06:40Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+	// SecretName is the name of a TLS secret.
+	//
+	// An existing tls secret.
+	SecretName string `json:"secretName"`


Suggested change

// SecretName is the name of a TLS secret.

//

// An existing tls secret.

SecretName string `json:"secretName"`

// SecretName is the name of the existing secret used to terminate TLS traffic.

SecretName string `json:"secretName"`

dprotaso · 2021-05-18T18:08:18Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+	// SecretNamespace is the namespace of the existing TLS secret.
+	//
+	// Namespace of the existing secret, if missing the namespace of the domainmapping is assumed.


Suggested change

// SecretNamespace is the namespace of the existing TLS secret.

//

// Namespace of the existing secret, if missing the namespace of the domainmapping is assumed.

// SecretNamespace is the namespace of the existing secret used to terminate SSL traffic. If empty the namespace of the DomainMapping is assumed.

dprotaso · 2021-05-18T18:10:26Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

+	// TLS indicates the existing tls secret that should be used for this domain.
+	//
+	// If defined it will use the tls secret with the given name assumed to exist
+	// in the same namespace as the domainmapping.


Suggested change

// TLS indicates the existing tls secret that should be used for this domain.

//

// If defined it will use the tls secret with the given name assumed to exist

// in the same namespace as the domainmapping.

// TLS allows the DomainMapping to terminate TLS traffic with an existing secret.

dprotaso · 2021-05-18T18:12:15Z

pkg/reconciler/domainmapping/reconciler.go

+	if dm.Spec.TLS != nil {
+		namespace := dm.Namespace
+		if dm.Spec.TLS.SecretNamespace != "" {
+			namespace = dm.Spec.TLS.SecretNamespace
+		}
+		return []netv1alpha1.IngressTLS{
+			{
+				Hosts:           []string{dm.Name},
+				SecretName:      dm.Spec.TLS.SecretName,
+				SecretNamespace: namespace,
+			},
+		}, nil, nil
+	}
+


Can you add a unit test for this as well?

dprotaso · 2021-05-18T20:10:43Z

test/e2e/autotls/domain_mapping_test.go

+	}, metav1.CreateOptions{})
+
+	if err != nil {
+		t.Fatalf("Domainmapping creation could not be completed: %v", err)


nit: update this message to say the secret failed to create

dprotaso · 2021-05-18T20:13:51Z

test/e2e/autotls/domain_mapping_test.go

+		Status: v1alpha1.DomainMappingStatus{},
+	}
+
+	t.Log("HOST===>", host)


unsure if this was for debugging - can we change the message to be a clear sentence or drop it?

dprotaso · 2021-05-18T20:15:01Z

test/e2e/autotls/domain_mapping_test.go

+	t.Log("HOST===>", host)
+	_, err = clients.ServingAlphaClient.DomainMappings.Create(context.Background(), &dm, metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("Problem creating domainmapping : %v", err)


Let's print the host here

Suggested change

t.Fatalf("Problem creating domainmapping : %v", err)

t.Fatalf("Problem creating DomainMapping %q: %v", host, err)

dprotaso · 2021-05-18T20:25:15Z

test/e2e/autotls/domain_mapping_test.go

+	if waitErr := wait.PollImmediate(test.PollInterval, test.PollTimeout, func() (bool, error) {
+		ingresses, err := clients.NetworkingClient.Ingresses.Get(context.Background(), ksvc.Service.Name, metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		for _, cond := range ingresses.Status.Conditions {
+			if cond.Type == apis.ConditionReady {
+				return cond.IsTrue(), nil
+			}
+		}
+		return false, nil
+	}); waitErr != nil {
+		t.Fatalf("Ingresses %s never became ready: %v", ksvc.Service.Name, waitErr)
+	}
+
+	err = shared.CheckDistribution(context.Background(), t, clients,
+		&url.URL{
+			Host:   host,
+			Scheme: "HTTPS",
+		}, 1, 1, []string{test.PizzaPlanetText1}, resolvableCustomDomain)
+	if err != nil {
+		t.Fatalf("Hitting %v didn't return the expected response: %v", dm.Name, err)
+	}


The Kingress is sorta an implementation detail of the DomainMapping - we should be checking the Ready condition on the DomainMapping itself

I also wonder if we CheckDistribution isn't needed and maybe we could just use WaitForEndpointState

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

allow domainmappings to specify the tls secret to be used by the autoTLS certificate Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Sameer Vohra <vsameer@vmware.com>

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

dprotaso · 2021-06-02T12:48:34Z

/approve 🎉

leaving lgtm to @julz

julz

Awesome stuff, thanks! Couple little indentation nits and then it'll lgtm 🔥

julz · 2021-06-02T12:53:07Z

pkg/reconciler/domainmapping/reconciler.go

+		return []netv1alpha1.IngressTLS{
+			{
+				Hosts:           []string{dm.Name},
+				SecretName:      dm.Spec.TLS.SecretName,
+				SecretNamespace: dm.Namespace,
+			},
+		}, nil, nil
+	}


indentation nit:

Suggested change

return []netv1alpha1.IngressTLS{

{

Hosts: []string{dm.Name},

SecretName: dm.Spec.TLS.SecretName,

SecretNamespace: dm.Namespace,

},

}, nil, nil

}

return []netv1alpha1.IngressTLS{{

Hosts: []string{dm.Name},

SecretName: dm.Spec.TLS.SecretName,

SecretNamespace: dm.Namespace,

}}, nil, nil

}

julz · 2021-06-02T12:58:29Z

test/e2e/domain_mapping_test.go

+	}
+
+	host := ksvc.Service.Name + ".example.org"
+	// set resolvabledomain for custom domain to false by default.


nit: not sure this comment really adds anything the code it's describing doesn't say more clearly, I'd probably just drop it

julz · 2021-06-02T13:02:10Z

test/e2e/domain_mapping_test.go

+				Name:       ksvc.Service.Name,
+				Namespace:  ksvc.Service.Namespace,
+				Kind:       "Service",
+			}, TLS: &v1alpha1.SecretTLS{


Suggested change

}, TLS: &v1alpha1.SecretTLS{

},

TLS: &v1alpha1.SecretTLS{

julz · 2021-06-02T13:14:35Z

test/e2e/domain_mapping_test.go

+
+	// service to be mapped


I'd be tempted to drop this comment since it doesn't really add anything, but if we keep it let's capitalise it and add a period just to be consistent with most of the rest of the code in serving.

Suggested change

// service to be mapped

// Create service to be mapped.

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

julz

/lgtm
/approve

🥳🥳🥳

knative-prow-robot · 2021-06-02T15:07:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, julz, shinigambit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso,julz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot · 2021-06-02T16:40:10Z

@shinigambit: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-serving-contour-tls	`5acb2a9`	link	`/test pull-knative-serving-contour-tls`
pull-knative-serving-kourier-stable-tls	`5acb2a9`	link	`/test pull-knative-serving-kourier-stable-tls`
pull-knative-serving-istio-stable-mesh-tls	`5acb2a9`	link	`/test pull-knative-serving-istio-stable-mesh-tls`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

leetcope · 2021-06-02T16:41:48Z

/retest

nak3 · 2021-07-30T04:30:22Z

It seems no documentation is available yet. I opened knative/docs#4081 to track for the new docs.

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 22, 2021

knative-prow-robot added area/API API objects and controllers area/networking size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2021

julz reviewed Apr 23, 2021

View reviewed changes

leetcope force-pushed the tls-byo-certificate branch from c8517ae to 6a26305 Compare April 24, 2021 05:53

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2021

leetcope force-pushed the tls-byo-certificate branch 2 times, most recently from 4e8d58c to 082a31e Compare April 24, 2021 06:21

knative-prow-robot assigned dprotaso May 3, 2021

julz suggested changes May 4, 2021

View reviewed changes

dprotaso reviewed May 4, 2021

View reviewed changes

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 6, 2021

leetcope force-pushed the tls-byo-certificate branch from b48d735 to e4b5958 Compare May 6, 2021 04:06

xtreme-sameer-vohra reviewed May 6, 2021

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2021

leetcope force-pushed the tls-byo-certificate branch from 5acb2a9 to 670109a Compare May 18, 2021 05:09

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 18, 2021

dprotaso reviewed May 18, 2021

View reviewed changes

leetcope force-pushed the tls-byo-certificate branch from 56aac5f to b14642a Compare May 21, 2021 17:31

leetcope and others added 13 commits June 1, 2021 14:30

fix domainmapping tls docs

76593e1

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

Domainmapping bring your own certificate

1aa6dfd

allow domainmappings to specify the tls secret to be used by the autoTLS certificate Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

fix linting problem

d9b7b08

Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

change domainmapping tls secret spec declaration

16dbce8

Co-authored-by: Bishoy Youssef <byoussef@vmware.com>

avoid kcert creation when tls secretName is provided

95c7ebe

Signed-off-by: Fabian Lopez <lfabian@vmware.com> Co-authored-by: Sameer Vohra <vsameer@vmware.com>

fix domainmapping tls docs

7637861

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

add e2e test for byo certificate

e2e6573

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

BYO cert: add e2e test and unit test

2154fb1

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

explicitly mark certificate as not required

a4f6a87

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

remove tls secret namespace field

9a7488b

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

add description to TLS provided externally message

1e95048

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

fix table test after rebase

56704ec

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

moved e2e domainmapping to be picked up by prow

ba2fd25

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

leetcope force-pushed the tls-byo-certificate branch from 90ab26a to ba2fd25 Compare June 1, 2021 20:07

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2021

julz reviewed Jun 2, 2021

View reviewed changes

fix code formatting

7461340

Signed-off-by: Fabian Lopez <lfabian@vmware.com>

julz approved these changes Jun 2, 2021

View reviewed changes

knative-prow-robot assigned julz Jun 2, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2021

knative-prow-robot merged commit f2cb8e5 into knative:main Jun 2, 2021

nak3 mentioned this pull request Jul 30, 2021

Support tls option for kn domain create knative/client#1408

Closed

nak3 mentioned this pull request Jul 21, 2023

DomainMappings can be configured with a Secret that doesn't exist in the same namespace #14183

Closed

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

	// Either a existing secret in the cluster or the new secret to be created.
	// Either an existing secret in the cluster or the new secret to be created.

	t.Fatalf("Problem creating domainmapping : %v", err)
	t.Fatalf("Problem creating DomainMapping %q: %v", host, err)

Conversation

leetcope commented Apr 22, 2021

Uh oh!

codecov bot commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leetcope commented May 3, 2021

Uh oh!

leetcope commented May 3, 2021

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SecretPrivateKeyMatchesSpec

SecretIssuerAnnotationsNotUpToDate

Uh oh!

dprotaso commented May 6, 2021

Uh oh!

dprotaso commented May 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 22, 2021 •

edited

Loading

dprotaso commented Jun 2, 2021 •

edited

Loading

knative-prow-robot commented Jun 2, 2021 •

edited

Loading